Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(examples): add deadline secrets management options to basic example #562

Merged
merged 1 commit into from
Sep 20, 2021
Merged

feat(examples): add deadline secrets management options to basic example #562

merged 1 commit into from
Sep 20, 2021

Conversation

kozlove-aws
Copy link
Contributor

Problem

Even though Secrets Management is on by default, we should update the basic example to have an optional value in the config for the admin credentials and steps in the readme.

Solution

Updated examples with new configurable parameters that allow to disable secret management (that is enabled by default) and provide own admin credentials.

Testing

Deployed updated examples and make sure that secret management is enabled and use provided credentials.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@jusiskin jusiskin added the contribution/core This is a PR that came from AWS. label Sep 9, 2021
@horsmand horsmand self-requested a review September 14, 2021 16:07
horsmand
horsmand suggested changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@horsmand horsmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, just a few small comments about some wording and formatting.

examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/app.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts Outdated Show resolved Hide resolved
horsmand
horsmand suggested changes Sep 16, 2021
View reviewed changes
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/app.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/lib/service_tier.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts Outdated Show resolved Hide resolved
@jericht jericht self-requested a review September 16, 2021 21:32
jericht
jericht requested changes Sep 17, 2021
View reviewed changes
Copy link
Contributor

@jericht jericht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just some minor comments

examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/lib/service_tier.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/bin/app.ts Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/bin/config.ts Outdated Show resolved Hide resolved
@jericht
Copy link
Contributor

jericht commented Sep 17, 2021

Also, can we make the commit message a bit more descriptive? How about something like:

feat(examples): add deadline secrets management options to basic example app

horsmand
horsmand reviewed Sep 20, 2021
View reviewed changes
Copy link
Contributor

@horsmand horsmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to go to the trouble of changing Secret to Secrets everywhere, we should get these ones as well.

examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/app.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/app.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/lib/service_tier.py Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/bin/app.ts Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/bin/config.ts Outdated Show resolved Hide resolved
horsmand
horsmand suggested changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@horsmand horsmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only see one last spot we missed changing secret to secrets. It also looks like a few of Jericho's previous comments haven't been addressed.

examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/lib/service_tier.py Outdated Show resolved Hide resolved
@kozlove-aws kozlove-aws force-pushed the examples_secret_manager branch 2 times, most recently from 46c0e17 to b39deae Compare September 20, 2021 19:56
@horsmand horsmand changed the title feat(secret): Add examples for secret manager feat(examples): add deadline secrets management options to basic example Sep 20, 2021
horsmand
horsmand suggested changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@horsmand horsmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to add on one last nitpicky thing at the end of the review, but it seems like it might affect the way the way the readme appears on GitHub.

examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md Outdated Show resolved Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/README.md Outdated Show resolved Hide resolved
@kozlove-aws kozlove-aws merged commit bd31a8d into aws:feature_enable_secret_manager Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@horsmand horsmand horsmand approved these changes

@jericht jericht jericht approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kozlove-aws @jericht @horsmand @jusiskin